This is not the appropriate place to define a class.

Maybe. I dont' want this class at all. The source comments give the impression that the Firmata application can do without servos, but than the examples do not build. Adding a new Servo.h to the library itself is also not a good idea because it will create conflicts.
So where put it? Into each example?

In the example, is it not possible to include ESP32Servo.h?

In theory, you could use the same switch as you are using elsewhere (i.e. ARDUINO_ARCH_ESP32).

I already considered using ESP32Servo.h. On first glance it should be compatible, but I did not try so far and I do not have the hardware to verify.

Problem I see here: it's an extra library that needs to be installed separately. So most users will choose the Firmata example, build and get a fail. Only some will read the docs and add the library first. It would be preferable to have the example work out of the box without this extra step and make it still an option. To do this the Servo.h include should be disabled and that can be done with the ARDUINO_ARCH_ESP32 define in the Firmata examples. The result could look like this:

_
#ifdef ARDUINO_ARCH_ESP32
// comment in if ESP32Servo library is installed and servos are required
//#include <ESP32Servo.h>
#else
#include <Servo.h>
#endif
_

What remains to do is to find a solution for the example code that depends on Servo.h. I know this is not ConfigurableFirmata, but previously I had to remove the servo code manually from the INO to make it work without (especially to save codespace for other purposes on small MCUs like the Atmega 328P). Here I see 2 options:

• use define ARDUINO_ARCH_ESP32
  advantage: compact binary
  disadvantage: ugly because several code blocks must be taken out
• use skeleton for Servo class, could be a file name FirmataServoSkeleton.h
  disadvantage: less compact binary

Problem I see here: it's an extra library that needs to be installed separately.

There is already a precedent for this behavior, because the regular sample requires the standard Servo library to be installed.

Please remove the class definition and proceed with the following:

#ifdef ARDUINO_ARCH_ESP32
  #include <ESP32Servo.h>
#else
  #include <Servo.h>
#endif

Done, see latest commit. I considered using a preprocessor check "#if __has_include("ESP32Servo.h)" to create a more descriptive error message if the library was not added, but this check is not a C standard and may cause problems.

@jnsbyr You can add a library dependency by adding a depends line to library.properties, as done here: https://github.com/firmata/ConfigurableFirmata/blob/master/library.properties Then the library manager should automatically grab the dependency.

Why are you creating an ESP32 specific API?

The ESP32 SDK does not provide the same functionality as the ESP8266 SDK. For some use cases it is still relevant to know if the stream is connected, as this is not the same as checking the WiFi status. One alternative would be to mimic the ESP8266 implementation and create a status value from the bool provided by the ESP32 SDK.

I think that's the way to go, especially since there is precedent.

I think that's the way to go.

You refer to the alternative I mentioned (to use the previous interface)?

Done, see latest commit. Also added an enum for the possible return values of method status() as the ESP32 SDK does not define constants like the ESP8266 SDK does.

Can you explain why this is necessary again?

Maybe we can figure out a creative solution to work around your need. My fear is that you are going to break backward compatibility with this change.

I had to do a similar change in ConfigurableFirmata, as the problem is that there's a conflicting definition of ANALOG (as well as INPUT and OUTPUT) in the ESP32 headers. IIRC, the values don't even match, so setting the pin mode could strangely fail, depending on which of the definitions is in scope at the calling place. But I think the right solution is to call this PIN_MODE_ANALOG, to avoid further ambiguities.

Does PIN_MODE_ANALOG come from Arduino.h?

Oh, I see it. It's in FirmataDefines.h. It seems like the right move is to move away from ANALOG and deprecate it altogether, as we did for INPUT and OUTPUT.

What is this doing for us? How does SERIAL_DEBUG get defined?

SERIAL_DEBUG can be found in StandardFirmataWifi.ino:90. If not enabled with ESP32 SDK (default) the compiler will raise a warning or an error (depending on the compiler settings) because of the unused local variables like "rssi" in line 898, caused by defining DEBUG_PRINT to nothing (see utility/firmataDebug.h).

Forgive me, I'm not familiar with the ESP8266 and ESP32 Wi-Fi libraries. Can you please explain the enum a little bit better?

Using this enum and the logic below, it looks like you are translating a boolean into values from the ESP8266 enum.

If that's correct, then I don't understand why you would want to redefine these values for ESP8266.

Shouldn't the enum only be defined under #if ESP32?

The ESP8266 SDK defines all the constants listed in the method description, but they do not exists with the same definition in the ESP32 SDK, if at all (they are related to the low level TCP socket implementation).

So similar how the Firmata library handles the PIN_MODE_XXX definitions I defined the enum to have the same definitions available, regardless which SDK is used. It would be possible to add all the constants known in the ESSP8266 SDK, but most of the other are typically of no practical value, so I added only the one that are available in both SDKs.

I hope that I got your initial idea right by implementing the status() method instead of the new connected() method to avoid having a platform specific solution. If not, just let me know.

Yes, I generally like your latest implementation. 👍

However, I'm concerned that you are "redefining" the symbols STATUS_CLOSED and STATUS_ESTABLISHED for the ESP8266 platform. They are already defined in the ESP8266 Wi-Fi library, no?

STATUS_CLOSED and STATUS_ESTABLISHED are enum values and are part of the WiFiStream class namespace of Firmata.

On the other hand CLOSED, LISTEN, etc. are defined as enum values in lwip/tcp.h of the ESP8266 SDK in the global namespace. So the two sets of constants have unique names and in the case oft ESP8266 SDK the same values. To make the code more robust to changes in the ESP8266 SKD one could use a switch:

inline uint8_t status()
{
  #ifdef ESP8266
    uint8 cs = _client.status();
    switch (cs)
    {
      case CLOSED:      cs = STATUS_CLOSED; break;
      case ESTABLISHED: cs = STATUS_ESTABLISHED; break;
      default: break;
    }
    return cs;
  #elif ESP32
    return _client.connected()? STATUS_ESTABLISHED : STATUS_CLOSED;
  #endif
}